-
Notifications
You must be signed in to change notification settings - Fork 616
Implement TRY_CAST #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement TRY_CAST #299
Conversation
Pull Request Test Coverage Report for Build 662686711
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @seddonm1
How does the release process work for this library? |
I'm embarrassed to say that I don't really know anymore. @Dandandan released a version more recently than me so maybe he or @maxcountryman can help with that. |
@andygrove I think we can let you off the hook for not knowing given the work you have done building DataFusion and Ballista 😁 |
thanks a lot, I will have a look at this PR later. |
Thanks @Dandandan |
Happy to help with a release. As @Dandandan mentioned, it should be automated once we've pushed a tag. |
@Dandandan are you able to review and hopefully release? This will allow us to add the CastOptions apache/arrow#9682 to DataFusion - hopefully before the next major release. |
@@ -201,6 +201,12 @@ pub enum Expr { | |||
expr: Box<Expr>, | |||
data_type: DataType, | |||
}, | |||
/// TRY_CAST an expression to a different data type e.g. `TRY_CAST(foo AS VARCHAR(123))` | |||
// this differs from CAST in the choice of how to implement invalid conversions | |||
TryCast { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering, why the choice TryCast
/ try_cast
?
I know bigquery uses SAFE_CAST
https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#safe_casting
It would make sense to support both maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could easily change this to add SAFE_CAST too. There was some rationale discussed here: apache/arrow#9682 (comment)
basically it was just that I think SQL Server got there first with TRY_CAST
I think this looks good @seddonm1 ! I think we are good to go, do you want to address the |
@seddonm1 will be released as 0.9.0 , thanks! |
Hi @andygrove
This change adds the TRY_CAST alongside CAST so that the choice on how to handle data casting errors can be differentiated.
See: https://docs.microsoft.com/en-us/sql/t-sql/functions/try-cast-transact-sql?view=sql-server-ver15
Also fixed a clippy error that must have been added recently.